Pull Request Test Coverage Report for Build 2260557767

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• 13 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.08%) to 89.48%

Totals
Change from base Build 2147526457:	-0.08%
Covered Lines:	1217
Relevant Lines:	1296

💛 - Coveralls

This Playground link shows why this is an issue well: https://playground.solidjs.com/?hash=865026612&version=1.3.9

There are two main problems:

1. The children are then compiled but never used, remaining in the build output, even after optimization/minification.
2. It's easy to not realize that the misplaced markup you're writing won't be rendered, the TypeScript compiler doesn't make any mention of it. This is in general against the spirit of TypeScript, since TypeScript should bubble up the underlying outwards facing API details as accurately as possible, in order to increase developer productivity and deliver a smooth user experience.

Based on further discussion on #typescript, I've changed this in two ways:

• Removed the shallow Readonly constraint on props
• Removed Props helper as it wasn't doing anything anymore.

One motivation for this change is that it would not play well with existing explicitly typed props, for example:

function MyComponent(props: {name: string}): JSX.Element { ... }

This type is not consistent with Component<any>, so in particular would not be a valid argument to lazy, for example, as illustrated in the previous need for this commit.

Another motivation for this change is that props might not be shallow Readonly: via spread props, it's possible to add a setter:

<A {...{ set a(){} }} />

Another motivation for this change is that props might not be shallow Readonly: via spread props, it's possible to add a setter

In my opinion this is a stretch. And while obviously it isn't stoppable I wouldn't give it any consideration or motivation here. 99.999% case shallow readonly is what you want. I can't speak to other implications so if it makes sense not to type that way I'm all for it. But the setter shouldn't be a consideration.

@high1 has proposed removing ComponentWithChildren (and PropsWithChildren) types, instead encouraging people to just list children?: JSX.Element in the argument to Component. The main motivation is that this is simpler, helping learn. Does anyone else have preferences either way?

I still think that component should be the default use case, which includes the composability of having children; like with HTML you get void elements which have no children. So I would prefer to have VoidComponents and VoidProps for this special case.

In my opinion, since children are something that not all components will need (really, the majority of them, mainly wrapper components that apply some styling or have their own local state), it makes more sense to write the names in an additive way, which would mirror the natural usage, such as ComponentWithChildren. The VoidComponent name doesn't seem as immediately obvious, it's not clear from the name what you're taking away from the regular Component type

I still think that component should be the default use case, which includes the composability of having children; like with HTML you get void elements which have no children. So I would prefer to have VoidComponents and VoidProps for this special case.

If Component implicitly adds children?: JSX.Element, then VoidComponent must be allowed to have children (similar to React.VFC), otherwise it is impossible to type components accepting non-JSX.Element children using one or the other. So they wouldn't be comparable to void elements which have no children (i.e. children?: never).

My opinion is that with these two types, it may be confusing to have Component for components that accept JSX.Element (and no other) children, and VoidComponent for components that accept no children, or non-JSX.Element children, if not already familiar with the react types.

... if not already familiar with the react types.

If you know JSX, chances are you are already familiar with the react types. And if not, we're not exactly clashing with preconceived expectations, are we?

FWIW, I know React well, but never used React with TypeScript. I started using TypeScript at roughly the same time I started using Solid. So it's all new to me, and I find React's types pretty confusing (and React seems to regret them). So I'm not convinced "same as React" is the right goal... By contrast, I'd argue that there's a natural notion of Component type that this PR adheres to.

But I also originally proposed adding VoidComponent (or a better name) — see "An alternative" in the PR description — before being convinced that this PR is the right way to go. So I'm not totally opposed to the idea, but I think we'd need a better argument than "same as React". (Perhaps "backwards compatibility is too important"? But it's just potential type errors, not functionality.)

@high1 has proposed removing ComponentWithChildren (and PropsWithChildren) types, instead encouraging people to just list children?: JSX.Element in the argument to Component. The main motivation is that this is simpler, helping learn. Does anyone else have preferences either way?

tbf I've never used the built-in types since I can just define my own props interface. I'm down with the Ref type though

@lxsmnsyc I suppose another option is to just deprecate Component (e.g. discourage it in the documentation). But it's unfortunate that there's no type then for "any component", which is what the existing Solid codebase means where it currently uses Component<any> (incorrectly without this PR).

@edemaine I don't think we should deprecate it. The main question is - should the Component type receive children by default, as a number of people expects, and I would say that's what the main purpose of a Component is, and if that's the case, VoidComponent type makes sense as a type that does not receive children. I don't think that there should be any assumptions which type is used more often. I also see the argument which comes from @otonashixav, that by unifying everything in one Component type which would require the user to be explicit with children declaration, and TBH, I'm starting to be more and more in favor of the latter, but I'm OK with any of these two. I don't have a very strong case against ComponentWithChildren, I just don't like the name and I feel that we would be more in line with what users are expecting by using Component and VoidComponent, favored by @atk, or be more on the edge, and simplify the typings by using one Component type which requires explicit children.

Based on more yet more discussion on #typescript with @atk and @otonashixav, I revised further to this plan:

• Component<T> type is just (props: T) => JSX.Element (no implicit children).
  • All component types below extend this
  • Component<T> extends Component<any> (or Component<any>) so this is now a workable "any component" type.
• VoidComponent<T> further asserts that there's no children in the props type. (Not sure this is crucial but it's nice for symmetry.)
• ParentComponent<T> is like our old Component, and has an optional children?: JSX.Element
• FlowComponent<T, C> has required children: C. This can be useful for building components that require a single-argument callback, for example.
• VoidProps, ParentProps, and FlowProps are helpers for typing just the props of such components.

I also tried to clean up some use of types in the code, and add some type exports to server code (I assume this is necessary to repeat here?). I'm not sure I found all places where these types could be used in the code, but I think it's an improvement.

• Component<T> extends Component which is equivalent to Component<{}> (no Component<any> needed). [Right? Why is CI failing?]

My bad, when I said that I only quickly tested in solid's playground (not a good place to test typescript) and assumed it would work the same as {}, but that's clearly not the case with function parameters, so Component<any> is still needed if <T>(props: Component<T>) => isn't viable.

function lazy<T>(
  fn: () => Promise<{ default: Component<T> }>
): Component<T> & { preload: () => Promise<{ default: Component<T> }> 

should work.

Thanks @otonashixav! For backward compatibility, I changed the definition of lazy back to use Component<any>. This preserves the notation lazy<typeof ChildComponent> which is used in the tests so maybe it's used by other code too...

Something just to add that I've seen throughout my React/Typescript years is the need to specify the Type of children. Will something like the FlowComponent cater for a <TabManager> component which does accept optional children, as long as they are of type <Tab>?

Will something like the FlowComponent cater for a <TabManager> component which does accept optional children, as long as they are of type <Tab>?

Yes, that's exactly the intent.

Something just to add that I've seen throughout my React/Typescript years is the need to specify the Type of children. Will something like the FlowComponent cater for a <TabManager> component which does accept optional children, as long as they are of type <Tab>?

When you invoke a component using JSX (<Tab>), the return type is always JSX.Element regardless of what the actual return type of <Tab> is. See microsoft/TypeScript#21699 and microsoft/TypeScript#29818. As such, it isn't possible to narrow the components allowed as children.

FlowComponent is used where the children can be something other than JSX.Element, for instance <For> and <Show> which accept functions which can benefit from the explicit typing.

@edemaine @otonashixav Is VoidComponent needed at all, then? All components would be void by default - and that seems to deprecate VoidComponent, doesn't it? I see no reason to have it as a separate type.

Is VoidComponent needed at all, then? All components would be void by default - and that seems to deprecate VoidComponent, doesn't it? I see no reason to have it as a separate type.

While you cannot call either directly with children, the former is still allowed to be assigned to a type that may accept children, whereas the former cannot since it never accepts children.

// note: this doesn't error in the playground because of strict: false
let MyComponent!: Component
let MyVoidComponent!: VoidComponent

let A: ParentComponent = MyComponent;
let B: ParentComponent = MyVoidComponent;
/*  ^ Type 'VoidComponent<{}>' is not assignable to type 'ParentComponent<{}>'.
  Types of parameters 'props' and 'props' are incompatible.
    Type '{ children?: Element; }' is not assignable to type '{ children?: undefined; }'.
      Types of property 'children' are incompatible.
        Type 'Element' is not assignable to type 'undefined'.
          Type 'null' is not assignable to type 'undefined'.(2322) */

Practically, this affects functions which accept a component e.g.: https://playground.solidjs.com/?hash=-853165744&version=1.3.13

@otonashixav I've looked at the playground, and it seems weird that the Dynamic accepts children property, when a plain component is referenced. Isn't the problem maybe that Dynamic uses current definitions which have implicit children?

That's probably why that specific example works in solid's playground. See https://tsplay.dev/w1yAKw instead.

The children will not be silently accepted on the Component - if not explicitly defined. I don't see a lot of value in explicitly forbidding them, when they first have to be opt-in. On the other hand, maybe having a convenience type is not bad...

Inspired by Otonashi's post, one context I could see using VoidComponent is in writing a higher-order component that takes a component as an argument, and it somehow doesn't want the component to accept children:

function wrapper(component: VoidComponent<any>) {
  // use component without giving it children
}

This seems like a pretty special case though, so I'm not sure we need to provide a type helper for it. I'm certainly happy to remove it if there's consensus; it does feel redundant. But as @atk suggested it, maybe he can comment?

It was meant as a convenience type for library developers to make sure users were not confused by components that don't render children. Since types usually don't influence the bundle size of resulting pages and apps, it shouldn't be an issue to provide them.

@atk @edemaine @otonashixav My main concern here is the user viewpoint - now we have a Component type which doesn't have implicit children, VoidComponent which explicitly forbids them, ParentComponent which brings back implicit children. How should a user type his component which doesn't accept children? Should he type it with Component or a VoidComponent - and is there a real difference in the most prevalent usage case? That's the main case for having one primitive type - but people could also find that too limiting and constrained.

We need to document these types (JSDoc; especially VoidComponent and ParentComponent need to be mentioned in Component's description). They could basically build VoidComponent themselves, but using a dedicated type which reuses the pattern of Void Elements in the HTML living standard is much more obvious and simple.

Not explicitly typing children as never only means losing a type error in a single unlikely scenario. For the most common case there isn't a difference.

Users are free to choose whatever suits them. They can define the types themselves without using any of the helpers, use only Component, or use all of the helpers. The extra types are named such that it is clear they are a special subset of components with extra semantic value, so I don't see it being difficult to decide which to choose. That said, if I had to argue for their removal:

• Libraries using the types force users to learn what the types do, if the names aren't self-descriptive to the person reading them
• VoidComponent may get misinterpreted as a component with a void return if you are not familiar with the definition, since that's what a void function is

For both of these good documentation and JSDoc will help.

I see that React has deprecated VoidFunctionComponent - and while I'm OK with keeping it here, adding these additional types only to the documentation would also serve the purpose, wouldn't it? The user could just copy the definitions for these if he feels he needs them, and we would avoid adding utility types to the core - types which are not used anywhere inside the codebase itself.

As I already remarked, it's a question of DX: if we already provide the type and the documentation, we save the developers some work while at the same time helping to make their types be as sound and useful as possible.

These types don't increase our bundle size noticably and will not even appear in the resulting app bundles, so what's not to like?

I also believe the react type was deprecated, because it lacked sufficient documentation, which led to confusion they sought to clean up by doing so. We can avoid the confusion to begin with, so that shouldn't be an issue.

The reason for deprecation is because it's equivalent with the FunctionComponent. As I said, I don't mind either solution, it's just that I don't see any notable benefit from adding Types which can be easily derived from Component.

From a "compatible naming" perspective, it does bother me a bit that VoidComponent in this PR is different from React's VoidComponent (which should really have been called Component -- and soon it will be), though of course our use is better 😃. Given the disagreement, perhaps it's better to remove VoidComponent (easier to add than remove)? I'm happy to go either way; I'd probably never need to use VoidComponent myself because Component (without a children prop) does the job when defining actual components.

I'm all on the side of adding only what's needed, maybe removing ParentComponent also, and having children optional in FlowComponent would be the best course?

Oops, didn't mean to open the can of worms that far. 😄

I feel like ParentComponent (which is the current Component) is the most used in practice today, so we should at least keep that one. I guess FlowComponent could be merged into it, adding a second parameter for the children type like the current FlowComponent.

On the other hand, I think ParentComponent and FlowComponent are each useful in different ways/contexts, and have very different semantic meaning too. (Whereas Component vs. VoidComponent seems like a less useful distinction.) In particular, forcing the existence of (nonoptional) children for FlowComponent seems useful for avoiding bugs.

I'm all on the side of adding only what's needed, ...

I would agree to this sentiment if we were talking about API surface – however this is about types, which are more documentation than code and in documentation, you should cover everything that is helpful to most people.

Thank you all for all your effort to get this one across the finish line.

I'm quite a bit late on this, but since SolidJS doesn't seem to follow semantic versioning, it's worth noting the change from Component to ParentComponent has broken some of my code and some from libraries I use, as Typescript no longer thinks they accept children.

Yeah TypeScript has been very difficult to keep with semantic versioning. This is partly because I don't think in TS and it takes some time for understanding the use cases in practice. Also TypeScript has been making updates that significantly improve typing story. In general these have been deemed worthwhile because of their impact and we've tried to keep them to minor versions.